From 816373d9cc2b941365b2feb1f6f6292d92174dc1 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 23 Oct 2014 10:38:44 -0700 Subject: [PATCH] Integrate the lockfile and registry-based deps This commit radically changes the approach of how lockfiles are handled in the package registry and how resolve interacts with it. Previously "using a lockfile" entailed just adding all of the precise sources to the registry and relying on them not being updated to remain locked. This strategy does not work out well for the registry, however, as one source can provide many many packages and it may need to be updated for other reasons. This new strategy is to rewrite instances of `Summary` in an on-demand fashion to mention locked versions and sources wherever possible. This will ensure that any relevant `Dependency` will end up having an exact version requirement as well as a precise source to originate from (if possible). This rewriting is performed in a few locations: 1. The top-level package has its dependencies rewritten to their precise variants if the dependency still matches the precise variant. This covers the case where a dependency was updated the the lockfile now needs to be updated. 2. Any `Summary` returned from the package registry which matches a locked `PackageId` will unconditionally have all of its dependencies rewritten to their precise variants. This is done because any previously locked package must remain locked during resolution. 3. Any `Summary` which points at a package which was not previously locked still has its dependencies modified to point at any matching locked package. This is done to ensure that updates are as conservative as possible. There are still two outstanding problems with lockfiles and the registry which this commit does not attempt to solve: * Yanked versions are not respected * The registry is still unconditionally updated on all compiles. --- src/cargo/core/dependency.rs | 23 +++-- src/cargo/core/registry.rs | 107 +++++++++++++++++++++-- src/cargo/core/source.rs | 5 +- src/cargo/core/summary.rs | 5 ++ src/cargo/ops/cargo_compile.rs | 6 +- src/cargo/ops/cargo_generate_lockfile.rs | 35 ++++---- src/cargo/ops/registry.rs | 2 +- src/cargo/ops/resolve.rs | 85 ++++++++++++++---- tests/test_cargo_registry.rs | 73 ++++++++++++++++ 9 files changed, 286 insertions(+), 55 deletions(-) diff --git a/src/cargo/core/dependency.rs b/src/cargo/core/dependency.rs index b57a3431b..edbfdb477 100644 --- a/src/cargo/core/dependency.rs +++ b/src/cargo/core/dependency.rs @@ -1,5 +1,6 @@ -use core::{SourceId, Summary}; use semver::VersionReq; + +use core::{SourceId, Summary, PackageId}; use util::CargoResult; /// Informations about a dependency requested by a Cargo manifest. @@ -112,6 +113,14 @@ impl Dependency { self } + /// Lock this dependency to depending on the specified package id + pub fn lock_to(mut self, id: &PackageId) -> Dependency { + assert_eq!(self.source_id, *id.get_source_id()); + assert!(self.req.matches(id.get_version())); + self.version_req(VersionReq::exact(id.get_version())) + .source_id(id.get_source_id().clone()) + } + /// Returns false if the dependency is only used to build the local package. pub fn is_transitive(&self) -> bool { self.transitive } pub fn is_optional(&self) -> bool { self.optional } @@ -122,12 +131,14 @@ impl Dependency { /// Returns true if the package (`sum`) can fulfill this dependency request. pub fn matches(&self, sum: &Summary) -> bool { - debug!("matches; self={}; summary={}", self, sum); - debug!(" a={}; b={}", self.source_id, sum.get_source_id()); + self.matches_id(sum.get_package_id()) + } - self.name.as_slice() == sum.get_name() && - (self.only_match_name || (self.req.matches(sum.get_version()) && - &self.source_id == sum.get_source_id())) + /// Returns true if the package (`id`) can fulfill this dependency request. + pub fn matches_id(&self, id: &PackageId) -> bool { + self.name.as_slice() == id.get_name() && + (self.only_match_name || (self.req.matches(id.get_version()) && + &self.source_id == id.get_source_id())) } } diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs index 171b73d83..cc0dd325c 100644 --- a/src/cargo/core/registry.rs +++ b/src/cargo/core/registry.rs @@ -1,4 +1,4 @@ -use std::collections::HashSet; +use std::collections::hashmap::{HashSet, HashMap, Occupied, Vacant}; use core::{Source, SourceId, SourceMap, Summary, Dependency, PackageId, Package}; use util::{CargoResult, ChainError, Config, human, profile}; @@ -21,10 +21,27 @@ impl Registry for Vec { } } +/// This structure represents a registry of known packages. It internally +/// contains a number of `Box` instances which are used to load a +/// `Package` from. +/// +/// The resolution phase of Cargo uses this to drive knowledge about new +/// packages as well as querying for lists of new packages. It is here that +/// sources are updated and (e.g. network operations) as well as overrides are +/// handled. +/// +/// The general idea behind this registry is that it is centered around the +/// `SourceMap` structure contained within which is a mapping of a `SourceId` to +/// a `Source`. Each `Source` in the map has been updated (using network +/// operations if necessary) and is ready to be queried for packages. pub struct PackageRegistry<'a> { sources: SourceMap<'a>, + config: &'a mut Config<'a>, + + // A list of sources which are considered "overrides" which take precedent + // when querying for packages. overrides: Vec, - config: &'a mut Config<'a> + locked: HashMap)>>>, } impl<'a> PackageRegistry<'a> { @@ -32,7 +49,8 @@ impl<'a> PackageRegistry<'a> { PackageRegistry { sources: SourceMap::new(), overrides: vec!(), - config: config + config: config, + locked: HashMap::new(), } } @@ -84,6 +102,18 @@ impl<'a> PackageRegistry<'a> { Ok(()) } + pub fn register_lock(&mut self, id: PackageId, deps: Vec) { + let sub_map = match self.locked.entry(id.get_source_id().clone()) { + Occupied(e) => e.into_mut(), + Vacant(e) => e.set(HashMap::new()), + }; + let sub_vec = match sub_map.entry(id.get_name().to_string()) { + Occupied(e) => e.into_mut(), + Vacant(e) => e.set(Vec::new()), + }; + sub_vec.push((id, deps)); + } + fn load(&mut self, source_id: &SourceId, is_override: bool) -> CargoResult<()> { (|| { let mut source = source_id.load(self.config); @@ -117,23 +147,86 @@ impl<'a> PackageRegistry<'a> { } Ok(ret) } + + // This function is used to transform a summary to another locked summary if + // possible. This is where the the concept of a lockfile comes into play. + // + // If a summary points at a package id which was previously locked, then we + // override the summary's id itself as well as all dependencies to be + // rewritten to the locked versions. This will transform the summary's + // source to a precise source (listed in the locked version) as well as + // transforming all of the dependencies from range requirements on imprecise + // sources to exact requirements on precise sources. + // + // If a summary does not point at a package id which was previously locked, + // we still want to avoid updating as many dependencies as possible to keep + // the graph stable. In this case we map all of the summary's dependencies + // to be rewritten to a locked version wherever possible. If we're unable to + // map a dependency though, we just pass it on through. + fn lock(&self, summary: Summary) -> Summary { + let pair = self.locked.find(summary.get_source_id()).and_then(|map| { + map.find_equiv(&summary.get_name()) + }).and_then(|vec| { + vec.iter().find(|&&(ref id, _)| id == summary.get_package_id()) + }); + + // Lock the summary's id if possible + let summary = match pair { + Some(&(ref precise, _)) => summary.override_id(precise.clone()), + None => summary, + }; + summary.map_dependencies(|dep| { + match pair { + // If this summary has a locked version, then we need to lock + // this dependency. If this dependency doesn't have a locked + // version, then it was likely an optional dependency which + // wasn't included and we just pass it through anyway. + Some(&(_, ref deps)) => { + match deps.iter().find(|d| d.get_name() == dep.get_name()) { + Some(lock) => dep.lock_to(lock), + None => dep, + } + } + + // If this summary did not have a locked version, then we query + // all known locked packages to see if they match this + // dependency. If anything does then we lock it to that and move + // on. + None => { + let v = self.locked.find(dep.get_source_id()).and_then(|map| { + map.find_equiv(&dep.get_name()) + }).and_then(|vec| { + vec.iter().find(|&&(ref id, _)| dep.matches_id(id)) + }); + match v { + Some(&(ref id, _)) => dep.lock_to(id), + None => dep + } + } + } + }) + } } impl<'a> Registry for PackageRegistry<'a> { fn query(&mut self, dep: &Dependency) -> CargoResult> { let overrides = try!(self.query_overrides(dep)); - if overrides.len() == 0 { + let ret = if overrides.len() == 0 { // Ensure the requested source_id is loaded try!(self.ensure_loaded(dep.get_source_id())); let mut ret = Vec::new(); for src in self.sources.sources_mut() { ret.extend(try!(src.query(dep)).into_iter()); } - Ok(ret) + ret } else { - Ok(overrides) - } + overrides + }; + + // post-process all returned summaries to ensure that we lock all + // relevant summaries to the right versions and sources + Ok(ret.into_iter().map(|summary| self.lock(summary)).collect()) } } diff --git a/src/cargo/core/source.rs b/src/cargo/core/source.rs index 15a9e5ae4..743c48f7f 100644 --- a/src/cargo/core/source.rs +++ b/src/cargo/core/source.rs @@ -143,9 +143,8 @@ impl SourceId { format!("git+{}{}{}", url, ref_str, precise_str) }, - SourceIdInner { kind: RegistryKind, .. } => { - // TODO: Central registry vs. alternates - "registry+https://crates.io/".to_string() + SourceIdInner { kind: RegistryKind, ref url, .. } => { + format!("registry+{}", url) } } } diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs index 4862572d5..12737fc6d 100644 --- a/src/cargo/core/summary.rs +++ b/src/cargo/core/summary.rs @@ -92,6 +92,11 @@ impl Summary { &self.features } + pub fn override_id(mut self, id: PackageId) -> Summary { + self.package_id = id; + self + } + pub fn map_dependencies(mut self, f: |Dependency| -> Dependency) -> Summary { let deps = mem::replace(&mut self.dependencies, Vec::new()); self.dependencies = deps.into_iter().map(f).collect(); diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index ef4464751..79ae34f55 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -90,7 +90,7 @@ pub fn compile_pkg(package: &Package, options: &mut CompileOptions) // First, resolve the package's *listed* dependencies, as well as // downloading and updating all remotes and such. - try!(ops::resolve_pkg(&mut registry, package)); + let resolve = try!(ops::resolve_pkg(&mut registry, package)); // Second, resolve with precisely what we're doing. Filter out // transitive dependencies if necessary, specify features, handle @@ -101,8 +101,8 @@ pub fn compile_pkg(package: &Package, options: &mut CompileOptions) let method = resolver::ResolveRequired(dev_deps, features.as_slice(), !no_default_features); let resolved_with_overrides = - try!(resolver::resolve(package.get_summary(), method, - &mut registry)); + try!(ops::resolve_with_previous(&mut registry, package, method, + Some(&resolve), None)); let req: Vec = resolved_with_overrides.iter().map(|r| { r.clone() diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/src/cargo/ops/cargo_generate_lockfile.rs index f789c3360..dc75ccbd1 100644 --- a/src/cargo/ops/cargo_generate_lockfile.rs +++ b/src/cargo/ops/cargo_generate_lockfile.rs @@ -28,7 +28,9 @@ pub fn generate_lockfile(manifest_path: &Path, let package = try!(source.get_root_package()); let mut config = try!(Config::new(shell, None, None)); let mut registry = PackageRegistry::new(&mut config); - let resolve = try!(ops::resolve_with_previous(&mut registry, &package, None)); + let resolve = try!(ops::resolve_with_previous(&mut registry, &package, + resolver::ResolveEverything, + None, None)); try!(ops::write_pkg_lockfile(&package, &resolve)); Ok(()) } @@ -51,46 +53,43 @@ pub fn update_lockfile(manifest_path: &Path, let mut config = try!(Config::new(opts.shell, None, None)); let mut registry = PackageRegistry::new(&mut config); + let mut to_avoid = HashSet::new(); + let mut previous = Some(&previous_resolve); - let mut sources = Vec::new(); match opts.to_update { Some(name) => { - let mut to_avoid = HashSet::new(); let dep = try!(previous_resolve.query(name)); if opts.aggressive { fill_with_deps(&previous_resolve, dep, &mut to_avoid, &mut HashSet::new()); } else { - to_avoid.insert(dep.get_source_id()); + to_avoid.insert(dep); match opts.precise { Some(precise) => { - sources.push(dep.get_source_id().clone() - .with_precise(Some(precise.to_string()))); + let precise = dep.get_source_id().clone() + .with_precise(Some(precise.to_string())); + try!(registry.add_sources(&[precise])); } None => {} } } - sources.extend(previous_resolve.iter() - .map(|p| p.get_source_id()) - .filter(|s| !to_avoid.contains(s)) - .map(|s| s.clone())); } - None => {} + None => { previous = None; } } - try!(registry.add_sources(sources.as_slice())); - let mut resolve = try!(resolver::resolve(package.get_summary(), - resolver::ResolveEverything, - &mut registry)); - resolve.copy_metadata(&previous_resolve); + let resolve = try!(ops::resolve_with_previous(&mut registry, + &package, + resolver::ResolveEverything, + previous, + Some(&to_avoid))); try!(ops::write_pkg_lockfile(&package, &resolve)); return Ok(()); fn fill_with_deps<'a>(resolve: &'a Resolve, dep: &'a PackageId, - set: &mut HashSet<&'a SourceId>, + set: &mut HashSet<&'a PackageId>, visited: &mut HashSet<&'a PackageId>) { if !visited.insert(dep) { return } - set.insert(dep.get_source_id()); + set.insert(dep); match resolve.deps(dep) { Some(mut deps) => { for dep in deps { diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 54b1b7759..2998dda1e 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -147,7 +147,7 @@ pub fn registry(shell: &mut MultiShell, })); let index = index.or(index_config).unwrap_or(RegistrySource::default_url()); let index = try!(index.as_slice().to_url().map_err(human)); - let sid = SourceId::new(RegistryKind, index.clone()); + let sid = SourceId::for_registry(&index); let api_host = { let mut config = try!(Config::new(shell, None, None)); let mut src = RegistrySource::new(&sid, &mut config); diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index c9ad81c05..51af6bcec 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -1,8 +1,8 @@ -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use semver::VersionReq; -use core::{MultiShell, Package}; +use core::{MultiShell, Package, PackageId}; use core::registry::PackageRegistry; use core::resolver::{mod, Resolve}; use core::source::Source; @@ -19,38 +19,89 @@ use util::profile; pub fn resolve_pkg(registry: &mut PackageRegistry, package: &Package) -> CargoResult { let prev = try!(ops::load_pkg_lockfile(package)); - let resolve = try!(resolve_with_previous(registry, package, prev.as_ref())); + let resolve = try!(resolve_with_previous(registry, package, + resolver::ResolveEverything, + prev.as_ref(), None)); try!(ops::write_pkg_lockfile(package, &resolve)); Ok(resolve) } -/// Resolve all dependencies for a package using an optional prevoius instance +/// Resolve all dependencies for a package using an optional previous instance /// of resolve to guide the resolution process. /// +/// This also takes an optional hash set, `to_avoid`, which is a list of package +/// ids that should be avoided when consulting the previous instance of resolve +/// (often used in pairings with updates). +/// /// The previous resolve normally comes from a lockfile. This function does not /// read or write lockfiles from the filesystem. -pub fn resolve_with_previous(registry: &mut PackageRegistry, - package: &Package, - previous: Option<&Resolve>) - -> CargoResult { +pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry, + package: &Package, + method: resolver::ResolveMethod, + previous: Option<&'a Resolve>, + to_avoid: Option<&HashSet<&'a PackageId>>) + -> CargoResult { let root = package.get_package_id().get_source_id().clone(); try!(registry.add_sources(&[root])); - match previous { + let summary = package.get_summary().clone(); + let summary = match previous { Some(r) => { - let v = r.iter().map(|p| p.get_source_id().clone()) - .collect::>(); - try!(registry.add_sources(v.as_slice())); + // In the case where a previous instance of resolve is available, we + // want to lock as many packages as possible to the previous version + // without disturbing the graph structure. To this end we perform + // two actions here: + // + // 1. We inform the package registry of all locked packages. This + // involves informing it of both the locked package's id as well + // as the versions of all locked dependencies. The registry will + // then takes this information into account when it is queried. + // + // 2. The specified package's summary will have its dependencies + // modified to their precise variants. This will instruct the + // first step of the resolution process to not query for ranges + // but rather precise dependency versions. + // + // This process must handle altered dependencies, however, as + // it's possible for a manifest to change over time to have + // dependencies added, removed, or modified to different version + // ranges. To deal with this, we only actually lock a dependency + // to the previously resolved version if the dependency listed + // still matches the locked version. + for node in r.iter().filter(|p| keep(p, to_avoid)) { + let deps = r.deps(node).into_iter().flat_map(|i| i) + .filter(|p| keep(p, to_avoid)) + .map(|p| p.clone()).collect(); + registry.register_lock(node.clone(), deps); + } + + let map = r.deps(r.root()).into_iter().flat_map(|i| i).filter(|p| { + keep(p, to_avoid) + }).map(|d| { + (d.get_name(), d) + }).collect::>(); + summary.map_dependencies(|d| { + match map.find_equiv(&d.get_name()) { + Some(&lock) if d.matches_id(lock) => d.lock_to(lock), + _ => d, + } + }) } - None => {} + None => summary, }; - let mut resolved = try!(resolver::resolve(package.get_summary(), - resolver::ResolveEverything, - registry)); + let mut resolved = try!(resolver::resolve(&summary, method, registry)); match previous { Some(r) => resolved.copy_metadata(previous), None => {} } - Ok(resolved) + return Ok(resolved); + + fn keep<'a>(p: &&'a PackageId, to_avoid: Option<&HashSet<&'a PackageId>>) + -> bool { + match to_avoid { + Some(set) => !set.contains(p), + None => true, + } + } } diff --git a/tests/test_cargo_registry.rs b/tests/test_cargo_registry.rs index c77b993a2..e72dd6f2e 100644 --- a/tests/test_cargo_registry.rs +++ b/tests/test_cargo_registry.rs @@ -2,6 +2,7 @@ use std::io::File; use support::{project, execs, cargo_dir}; use support::{UPDATING, DOWNLOADING, COMPILING, PACKAGING, VERIFYING}; +use support::paths::PathExt; use support::registry as r; use hamcrest::assert_that; @@ -218,3 +219,75 @@ version required: ^0.0.1 dir = p.url(), ))); }) + +test!(lockfile_locks { + let p = project("foo") + .file("Cargo.toml", r#" + [project] + name = "foo" + version = "0.0.1" + authors = [] + + [dependencies] + bar = "*" + "#) + .file("src/main.rs", "fn main() {}"); + p.build(); + + r::mock_pkg("bar", "0.0.1", []); + + assert_that(p.process(cargo_dir().join("cargo")).arg("build"), + execs().with_status(0).with_stdout(format!("\ +{updating} registry `[..]` +{downloading} bar v0.0.1 (the package registry) +{compiling} bar v0.0.1 (the package registry) +{compiling} foo v0.0.1 ({dir}) +", updating = UPDATING, downloading = DOWNLOADING, compiling = COMPILING, + dir = p.url()).as_slice())); + + p.root().move_into_the_past().unwrap(); + r::mock_pkg("bar", "0.0.2", []); + + assert_that(p.process(cargo_dir().join("cargo")).arg("build"), + execs().with_status(0).with_stdout(format!("\ +{updating} registry `[..]` +", updating = UPDATING).as_slice())); +}) + +test!(lockfile_locks_transitively { + let p = project("foo") + .file("Cargo.toml", r#" + [project] + name = "foo" + version = "0.0.1" + authors = [] + + [dependencies] + bar = "*" + "#) + .file("src/main.rs", "fn main() {}"); + p.build(); + + r::mock_pkg("baz", "0.0.1", []); + r::mock_pkg("bar", "0.0.1", [("baz", "*")]); + + assert_that(p.process(cargo_dir().join("cargo")).arg("build"), + execs().with_status(0).with_stdout(format!("\ +{updating} registry `[..]` +{downloading} [..] v0.0.1 (the package registry) +{downloading} [..] v0.0.1 (the package registry) +{compiling} baz v0.0.1 (the package registry) +{compiling} bar v0.0.1 (the package registry) +{compiling} foo v0.0.1 ({dir}) +", updating = UPDATING, downloading = DOWNLOADING, compiling = COMPILING, + dir = p.url()).as_slice())); + + p.root().move_into_the_past().unwrap(); + r::mock_pkg("baz", "0.0.2", []); + r::mock_pkg("bar", "0.0.2", [("baz", "*")]); + + assert_that(p.process(cargo_dir().join("cargo")).arg("build"), + execs().with_status(0).with_stdout(format!("\ +{updating} registry `[..]` +", updating = UPDATING).as_slice())); +}) -- 2.30.2